-
Notifications
You must be signed in to change notification settings - Fork 29k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
src: remove calls to deprecated v8 functions (Uint32Value) #22143
Conversation
src/node_buffer.cc
Outdated
Local<Context> ctx = env->context(); | ||
uint32_t start, end; | ||
if (!args[2]->Uint32Value(ctx).To(&start) || | ||
!args[3]->Uint32Value(ctx).To(&end)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can use .As<Uint32>()
for both arguments here. They are coerced in JS with value >>> 0
.
src/node_buffer.cc
Outdated
@@ -1003,7 +1009,7 @@ void IndexOfNumber(const FunctionCallbackInfo<Value>& args) { | |||
THROW_AND_RETURN_UNLESS_BUFFER(Environment::GetCurrent(args), args[0]); | |||
SPREAD_BUFFER_ARG(args[0], ts_obj); | |||
|
|||
uint32_t needle = args[1]->Uint32Value(); | |||
uint32_t needle = args[1].As<v8::Uint32>()->Value(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: add using v8::Uint32;
at the top of the namespace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, if you are sure this is a Uint32, modify the CHECK above to IsUint32
src/node_crypto.cc
Outdated
static_cast<point_conversion_form_t>(args[0]->Uint32Value()); | ||
uint32_t val; | ||
if (!args[0]->Uint32Value(env->context()).To(&val)) | ||
return env->ThrowError("Failed to parse argument"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to throw an error here?
src/node_crypto.cc
Outdated
static_cast<point_conversion_form_t>(args[2]->Uint32Value()); | ||
uint32_t val; | ||
if (!args[2]->Uint32Value(env->context()).To(&val)) | ||
return env->ThrowError("Failed to parse argument"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
src/tcp_wrap.cc
Outdated
@@ -180,7 +180,7 @@ void TCPWrap::SetKeepAlive(const FunctionCallbackInfo<Value>& args) { | |||
args.Holder(), | |||
args.GetReturnValue().Set(UV_EBADF)); | |||
int enable = args[0]->Int32Value(); | |||
unsigned int delay = args[1]->Uint32Value(); | |||
unsigned int delay = args[1].As<v8::Uint32>()->Value(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This value is not verified in JS
a80ba86
to
3e400a2
Compare
Ping. This is ready for review. |
src/node_buffer.cc
Outdated
int value = args[1]->Uint32Value() & 255; | ||
memset(ts_obj_data + start, value, fill_length); | ||
uint32_t val; | ||
if (args[1]->Uint32Value(ctx).To(&val)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fwiw, I still find it easier to read when using the return-on-error style rather than an if (success) { … }
block
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Me too. Fixed.
Remove all calls to deprecated v8 functions (here: Value::Uint32Value) inside the code (src directory only).
It looks like the value is not always an integer... let me check |
I'm lost here... It seems to me that I added this just before the binding is called: process._rawDebug(`${start}, ${end}`);` Last few lines of output:
Here is the relevant code: Lines 869 to 888 in 59e5a39
|
Here is the failing block: node/test/parallel/test-buffer-fill.js Lines 320 to 325 in 59e5a39
|
I don’t know if it’s relevant, but there seems to be a bunch of compiler warnings from this PR:
|
@targos I think this is the failing block: node/test/parallel/test-buffer-fill.js Lines 327 to 330 in 59e5a39
|
@addaleax Haha of course, thanks! I think I completely forgot to run the tests locally on this PR (even before making my own changes), sorry. This should be fixed now. |
0163064
to
0bf15c5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still LGTM :)
Environment* env = ctx->env(); | ||
Local<Context> context = env->context(); | ||
|
||
unsigned int flush; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess CI is going to tell us whether it’s a real problem, but at least in theory this would be uint32_t
, too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Though perhaps it's worth adding a macro for converting to uint32?
something like (not sure for the exact macro as I don't have a lot of experience with them and they are quite tricky)
#define CHECKED_TO_UINT32(ctx, from, variable) \
uint32_t variable; \
if (!(from)->Uint32Value((ctx)).To(&variable)) return;
CHECKED_TO_UINT32(ctx, args[2], start);
(Though this is not needed if we go with FromMaybe
version)
uint32_t start; | ||
if (!args[2]->Uint32Value(ctx).To(&start)) return; | ||
uint32_t end; | ||
if (!args[3]->Uint32Value(ctx).To(&end)) return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure but perhaps FromMaybe
version is clearer?
uint32_t start = args[2]->Uint32Value(ctx).FromMaybe(0);
uint32_t end = args[3]->Uint32Value(ctx).FromMaybe(0);
Though this will result in function actually continuing execution if provided with invalid value (but wasn't this how it was before - Uint32Value()
returns 0 on invalid values AFAIK? )?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though this will result in function actually continuing execution if provided with invalid value
That is the previous behaviour, which is buggy in that it will swallow exceptions if both calls fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 shouldn't this then be semver-major to avoid possible breakage?
Also, perhaps we can
if (!args[2]->IsUint32() || !args[3]->IsUint32())
return args.GetReturnValue().Set(-2);
uint32_t start = args[2].As<Uint32>()->Value();
uint32_t end = args[3].As<Uint32>()->Value();
in this case (tests seem to pass)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only place where we pass in invalid arguments is from a process.binding()
test. We could/should probably remove that test in a follow-up PR, and revert 05aa50f (which is pretty close to your suggestion)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I thought that this was expected somewhere else, therefore the test. The suggested commit is even better, with the above I wanted to somehow mitigate that test.
Remove all calls to deprecated v8 functions (here: Value::Uint32Value) inside the code (src directory only). PR-URL: #22143 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Remove all calls to deprecated v8 functions (here: Value::Uint32Value) inside the code (src directory only). Co-authored-by: Michaël Zasso <targos@protonmail.com> PR-URL: nodejs#22143 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Landed in 167dd36 |
Remove all calls to deprecated v8 functions (here: Value::Uint32Value) inside the code (src directory only). PR-URL: #22143 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Remove all calls to deprecated v8 functions (here: Value::Uint32Value) inside the code (src directory only). PR-URL: #22143 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Remove all calls to deprecated v8 functions (here: Value::Uint32Value) inside the code (src directory only). PR-URL: #22143 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Remove all calls to deprecated v8 functions (here:
Value::Uint32Value) inside the code (src directory only).
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes/cc @addaleax @hashseed